-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for upgraded DCMTK and web-assembly build #496
Conversation
This work might be related, @michaelonken ran into some issues that have not been resolved: #493. @rfloca would you like to check this out to see if this creates any issues for MITK? |
For the reference, here's configuration of DCMTK for Slicer: https://github.com/Slicer/Slicer/blob/main/SuperBuild/External_DCMTK.cmake DCMTK apps selected https://github.com/Slicer/Slicer/blob/main/CMake/SlicerBlockInstallDCMTKApps.cmake#L5 |
MITK DCMTK config: https://github.com/MITK/MITK/blob/master/CMakeExternals/DCMTK.cmake (right now uses DCMTK-3.6.7 from upstream https://github.com/MITK/MITK/blob/master/CMakeExternals/DCMTK.cmake#L33-L34). |
We are seeing Windows segfaults on other packages on GitHub Actions starting this week-ish. One approach that may help is the CMake minimum version bump re::
Edit: related commit: InsightSoftwareConsortium/ITK@4b010e7 |
@jadh4v @thewtex @michaelonken let's try to use this discord channel for coordination: https://discord.gg/3vZakgNS |
Checking to see if this would address Win runtime error in QIICR#496
FYI re:
The ITK-Wasm DCMQI / DCMTK modules address this issue without any changes needed to DCMTK. Access in the Wasm runtime is contained only to the directories of the files passed. |
I'll do. But it might take 2-3 weeks. Currently is a lot todo and I have to see when I find the time. Please excuse that I cannot react faster. |
I have locally tested the super-build and tests on windows 11 VS2022. All tests are passing. Ran into a build issue regarding DCMTK targets. I have a temp fix for it locally, but will consult with others for more appropriate fix. |
After discussing with @jadh4v , I suggest we ensure dcmqi can support being built against a newer DCMTK without updating the version currently used in its external project. In the short term, this will allow ITK wasm to build dcmqi against a newer version of DCMTK. In the medium term (likely during the upcoming Slicer project week), we will work on:
The includes of headers available only in newer DCMTK could be made conditionally after including cc: @michaelonken |
Checking to see if this would address Win runtime error in QIICR#496
per suggestion from @thewtex, see actions/runner-images#10004
(This is a work in progress)
Updates to DCMQI library after upgrading DCMTK to a later commit (InsightSoftwareConsortium/DCMTK@0c72275).
Upgrade CMake minimum version to better support sub-project mode when overriding default CMAKE variable values.
These updates are related to the effort to add DICOM SEG Object read/write capabilities in ITK-wasm.
Current DCMTK fork used by ITK-wasm (includes patches for web assembly build): https://github.com/InsightSoftwareConsortium/DCMTK/tree/20240311_DCMTK_PATCHES_FOR_ITK-wasm